feat(cli): add ACP session creation and listing - #3744
Conversation
67c6644 to
4ed9973
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
4ed9973 to
951bd81
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for adding the ACP session lifecycle in a focused layer. I found two production-path issues that look worth addressing before this moves forward. These are suggestions from an outside review, so please do push back if an ACP lifecycle or bootstrap guarantee changes either premise.
AI-assisted review disclosure: Codex ran independent protocol, lifecycle, and test analysis lanes; Astro-Han is the contributor of record for this review.
|
|
||
| let subscription: AcpSessionSubscription; | ||
| try { | ||
| subscription = await connection.openSessionSubscriptionOnce({ |
There was a problem hiding this comment.
[P1] (category ① — normal supported path)
Thanks for keeping Runtime Host as the Session authority. One lifecycle limit still looks reachable on the normal session/new path: this creates the durable Session first, then opens and retains one subscription for every created Session until the entire ACP stdio connection exits. Runtime Host caps a connection at 16 subscriptions, and this PR has no per-Session close/release operation, so the 17th create returns an error after the Session was already committed; each retry creates another catalog entry and fails again. Could the adapter either make attachment lazy/releasable, or reserve capacity with the real subscription owner before committing the Session, and cover 17+ creates on one real ACP connection? Please feel free to push back if the ACP client lifecycle guarantees the process is replaced before this limit.
There was a problem hiding this comment.
Verified: the 17th create could reach session.create before the Host rejected subscription.open, leaving a durable Session behind. Fixed in 419c386 without changing the approved PR2 lifecycle boundary. The registry now reserves the shared Runtime Host per-connection subscription capacity from active + pending attachments before session.create, including concurrent requests; the 17th request is rejected before persistence, and capacity is released when a subscription terminates. The Host and adapter use one shared limit constant. Coverage includes concurrent admission, released-slot reuse, and 17 creates through the official ACP SDK against a real Runtime Host.
I am pushing back on making attachment lazy/releasable or adding a connection pool in this PR: those would conflict with PR2s one shared Host context, immediate continuous subscription consumption, and deliberately unimplemented session/close. PR2 therefore keeps a maximum of 16 simultaneously attached Sessions; broader live-session support should be revalidated with the planned close lifecycle rather than changing that architecture here.
| ): Promise<number> { | ||
| const sessionRegistry = new AcpSessionRegistry({ | ||
| connect: async (signal) => { | ||
| const context = await (dependencies.connectRuntimeHostCli ?? connectRuntimeHostCli)({ |
There was a problem hiding this comment.
[P2] (category ② — reasonable failure/recovery path)
Thanks for reusing the existing Host bootstrap. The ACP registry only consumes the connection and close(), but connectRuntimeHostCli() also reads the entire connection/model catalog before returning. That makes otherwise healthy session/list and session/new fail as connection_failed whenever the unrelated connection catalog is temporarily unreadable or revision-churning, before a Session request is sent. Would a connection-only/lazy-catalog bootstrap be a better fit here, with a regression where the connection catalog fails while session.catalog.query remains healthy? Please push back if ACP intentionally requires a healthy model catalog even for listing existing Sessions.
There was a problem hiding this comment.
Verified: ACP session/new and session/list do not require the model connection catalog, so the previous bootstrap coupled healthy Session operations to an unrelated catalog read. Fixed in 419c386 by extracting connectRuntimeHostCliConnection for connection/profile ownership and cleanup; the existing connectRuntimeHostCli remains the catalog-reading wrapper for current CLI/TUI callers, while ACP uses the connection-only seam. A regression makes the model catalog reader throw and confirms the connection-only bootstrap still succeeds; existing catalog abort/cleanup behavior remains covered.
Generated-by: Codex
Generated-by: Codex
…fecycle # Conflicts: # packages/cli/src/runtime-host-cli-context.ts
me2seeks
left a comment
There was a problem hiding this comment.
Approving. Reviewed the registry, the Runtime Host seams, and the ACP wiring; boundaries hold up.
Verified explicitly:
- cwd filtering is canonical on both sides.
session/newstoresworkspaceResolver's realpath-canonicalized cwd, and#listrealpaths the client-supplied filter before comparing againsthostCwd— so a session created through this adapter is not filtered out of its ownsession/liston symlinked paths (macOS/tmpetc.). - Legacy omission is exact.
'kind' in sessiononly matchesunsupported_legacy_record;SessionCatalogProjectionand the separate shared-session projection are unaffected. - Subscription lifecycle is race-safe. Dispose-during-connect and dispose-during-open both close the late-arriving resource;
#records.setand the closing check are synchronous-adjacent so no record can register after disposal begins. Capacity accounting (#activeSubscriptions + #pendingSubscriptionOpens) mirrors the Host's ownSESSION_CONNECTION_SUBSCRIPTION_MAX_ITEMS = 16admission, and client-generated session ids makesession.createretries collide safely (operation_conflict/ existing-record probe). - One-shot subscription open on the reconnecting connection (
#requireCurrent('subscription.open')) correctly refuses to silently land on a replacement Host epoch. - Cursor codec is strict: versioned, size-bounded, canonical base64url, revision format-checked, and
cursor_cwd_mismatchrejects cross-filter reuse before any Host I/O. connectRuntimeHostCliConnectionsplit also fixes a pre-existing leak:initialConnectionis now closed if reconnecting-connection setup fails, andacquireAbortablycloses a connection that resolves after abort.
Non-blocking notes (P3, no action needed):
#recordsretains EOF'd/failed subscriptions forinspect()for the lifetime of the ACP connection — slow unbounded growth across manysession/newcalls in one editor session, but each entry is small (transcript: none) and nothing reads them on the hot path.- A subscription that dies mid-session is recorded on the record but never surfaces to the ACP client (no notification exists yet). Fine for this PR's scope (new/list only); worth keeping in mind when
session/promptlands, since a dead subscription will then be user-visible.
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "epoch": 87, | |||
There was a problem hiding this comment.
Is this conflict with main already?
There was a problem hiding this comment.
Yes — good catch. This was a semantic conflict rather than a textual merge conflict: main advanced from epoch 87 to 93 after the previous alignment, so Git could merge cleanly while this compatible-change declaration became stale.
Fixed in 7b1165b after merging the latest main; the declaration now names epoch 93. That merge also exposed two CLI test fixtures missing the new subscribeConnectionCatalogChanges seam from main, fixed in 081e120.
The protocol epoch guard, typecheck, lint, format check, and the focused ACP/Runtime Host suite (99 tests) pass locally. CI is running on the updated branch.
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the work on this one. The two findings from my first pass are properly fixed, and me2seeks' checklist holds up under a second read: cwd canonicalisation lives in the Host's workspace resolver, the capacity reservation is a synchronous check-then-increment so 17 concurrent creates cannot race it, dispose-during-connect and dispose-during-open both close the late resource, and the one-shot open never lands on a replacement Host. Nice.
What I want to raise is one design question, because I think it decides whether this PR lands as a net simplification or as a new constraint the next PRs have to live with.
Does PR2 need to attach a subscription at session/new? I went looking for what the subscription buys in this PR and could not find a reader. record.snapshot and record.failure are only read by inspect(), and every caller of inspect() is in the tests. On the Host side I checked what happens with zero subscribers: the coordinator drops its in-memory projection (#sessions.delete) and nothing else; the durable Session is untouched. So a session/new that only does session.create and returns the id is a complete PR2.
If attach moves to the first session/prompt in PR3, which is also ACP's natural shape (clients create many sessions and prompt a few), these all go with it: the 16-session ceiling and its -32603 (a ceiling the description does not mention today, and one an editor user can only clear by restarting the agent), #reserveSubscriptionCapacity and the pending/active counters, #consume, inspect() and its two exported types, the #records retention that me2seeks flagged, the SESSION_CONNECTION_SUBSCRIPTION_MAX_ITEMS protocol export, the compatible-change declaration (and with it the epoch question in the other thread), and roughly 220 lines of tests that exist to observe inspect(). The registry ends up about half its current size, and PR3 gets to decide the attach lifecycle with a real consumer in hand.
I know you pushed back on lazy attachment earlier as conflicting with PR2's boundary. My read is that the boundary is the thing under question: I could not find a constraint that requires attaching at create time, only the decision to do so. If there is one I have missed, say so and I will drop this.
If you would rather keep the attach, that is a legitimate call for PR2, and then three small things before approval: the declaration's epoch must equal main's at merge time (93 today; it will drift again, so set it last), maka --help at cli-core.ts:136 still says "initialize only", and the 16-session ceiling should be stated in the description and in that help line.
Either way, a few things that are independent of the decision, inline below and here:
readRuntimeHostSessionsnow builds on the single-page reader, but onlyreadNextcatchesrevision_changed;readFirstletsRuntimeHostSessionCatalogRevisionChangedErrorescape to callers that only knowRuntimeHostCatalogReadError. Unreachable today becauselist_startsends no revision, but a two-line catch keeps that structural rather than Host-dependent.- ACP's schema has
mcpServersas a required field onsession/newand no capability bit for "no MCP at all", so any editor with a context server configured gets-32602from this adapter. Not a PR2 problem since prompt is not here yet, but it needs an answer before PR3; if you have a Zed handy, one realsession/newwould settle what clients actually send. - The cursor codec's base64url canonicalisation and the exact-four-keys check are at odds with the
vversion field: either the cursor is opaque and any foreign byte is rejected (thenvis dead), orvis the compatibility mechanism (then the two checks should go). One or the other.
Evidence boundary: static read of 711045e6 against main 6c632b13; maka-agent (737) and @maka/runtime-host (1555) suites green locally; the epoch guard was run on a locally built merge result and exits 2 as expected. No real editor was connected.
AI-assisted review: drafted with Maka; I verified the zero-subscriber Host path, the inspect() consumer search, and the guard result myself.
简体中文
上一轮两条都修好了,me2seeks 核过的点复核成立。主要想提一个设计问题:PR2 在 session/new 时 attach 的 subscription 在本 PR 里没有任何生产读者,Host 侧零订阅者也只丢内存投影、不影响 durable Session。如果把 attach 推迟到 PR3 的首次 prompt,16 上限、容量预留、inspect()、协议导出、兼容声明和 epoch 问题一起消失,registry 瘦一半。如果你还是想在 PR2 保留 attach,那 approve 前需要:epoch 改成合并时 main 的值、--help 文案更新、16 上限写进正文。其余几条与决定无关,见行内。
|
|
||
| let subscription: AcpSessionSubscription; | ||
| try { | ||
| subscription = await connection.openSessionSubscriptionOnce({ |
There was a problem hiding this comment.
This is the attach. Nothing in PR2 reads what it produces (snapshot and failure only feed inspect(), which only tests call), and the Host keeps the durable Session with zero subscribers. If it moves to PR3's first prompt, everything from L205 down to #consume goes with it.
| this.#activeSubscriptions += 1; | ||
| record.consumerTask = this.#consume(record); | ||
| return { sessionId }; | ||
| } finally { |
There was a problem hiding this comment.
If the reservation stays: this finally is the half of the P1 fix nothing pins. Delete it and the suite is still green, because the failure cases each do one create and then dispose. Sixteen successful creates after the failing one would lock it.
| } | ||
| } | ||
|
|
||
| #reserveSubscriptionCapacity(): void { |
There was a problem hiding this comment.
If the reservation stays: this mirrors the Host's per-connection admission with a compile-time constant, and it is only right while the registry owns its connection alone. Worth one sentence here saying so, since the day ACP shares a Host connection with another CLI path the client count drifts and we are back to the original P1.
| input: SubscriptionOpenInput, | ||
| timeoutMs?: number, | ||
| ): Promise<RuntimeHostSessionSubscription> { | ||
| return this.openSessionSubscription(input, timeoutMs); |
There was a problem hiding this comment.
On a concrete connection "once" has no meaning; this body is openSessionSubscription verbatim, and the only real semantics live in the reconnecting layer's #requireCurrent. It landed on the base interface because runtime-host-cli-context.ts:90 types the field as RuntimeHostConnection while it always holds the reconnecting one. Narrow that field and this method can stay on the reconnecting connection only; the "composes through a decorated connection" test goes with it.
Keep PR2 limited to durable session creation and listing, and move live subscription attachment to the first prompt in PR4. Normalize first-page catalog revision changes and tighten ACP cursor validation. Generated-by: Codex
|
Thanks for the review. I’ve revised PR2 to keep it limited to durable session creation and listing. PR3 will expose the Runtime Host-backed Session configuration through ACP. It will add lightweight current-connection ownership tracking when that state gets its first real consumer, return the supported Subscription attachment is deferred to the first 感谢 review。我已调整 PR2,使其范围仅限于 durable session 的创建和列出。 PR3 将通过 ACP 暴露由 Runtime Host 管理的 Session 配置能力。届时 ownership 状态会出现首个实际消费者,因此 PR3 会引入轻量的 current-connection ownership tracking;同时让 Subscription attach 延至 PR4 的首次 |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving 7cbd25e9. This is the PR2 I was hoping for: session/new is session.create and an id, session/list is a filtered page over the catalog reader, and the registry is down from 571 to 399 lines. The subscription machinery is gone without residue: no capacity reservation, no inspect(), no SESSION_CONNECTION_SUBSCRIPTION_MAX_ITEMS export, no compatible-change declaration, no alias on the connection, and protocol/, server/, connection.ts and reconnecting-connection.ts are byte-identical to main. So the epoch question dissolves too. The seventeen-session proof runs against a real Host in a child process, and the stdio-server test drives session/new through a connection that has no subscription method at all, which is the structural version of the same fact. readFirst and readNext now share one revision-changed retry, the cursor is opaque with v dropped and the exact-key check kept, and the help line matches. Merge-tree against main is clean, CI is green.
One thing to carry into PR3 rather than fix here: mcpServers still returns -32602 when non-empty, and the ACP schema makes the field required. Before session/prompt lands, one real session/new from Zed or another editor with a context server configured would settle what clients actually send.
Two small ones, take or leave: the in-process seventeen-session test asserts subscriptionOpens === 0 on a counter nothing in the registry can increment, so it cannot fail; the child-process and stdio-server tests already carry that obligation and this one can go, along with the last openSessionSubscriptionOnce string in the tree. And session/list can return an empty page with a nextCursor when client-side cwd filtering empties a Host page, which is fine by ACP but worth one sentence in the body.
Evidence boundary: static read of 7cbd25e9 against main 92fa5281; affected test:dist files run for maka-agent and @maka/runtime-host (33 + 57 + 39 passing), lint and format clean; no editor client connected.
AI-assisted review: drafted with Maka; I verified the deletions against main, the child-process proof and the merge result myself.
简体中文
批准合并。PR2 收成了 create + list,subscription 相关机制全部删净,协议、server 和 connection 与 main 逐字一致,epoch 问题随之消失。17 session 的证明跑在真实 Host 子进程上。mcpServers 非空仍返回 -32602,这是 PR3 之前要回答的问题,最好拿一个真实编辑器的 session/new 定下来。两条可选:in-process 那条 17 session 测试的断言不可能失败,可删;session/list 可能返回空页带 cursor,正文点一句。
|
|
||
| assert.equal(creates.length, sessionCount); | ||
| assert.equal(createdSessionIds.length, sessionCount); | ||
| assert.equal(subscriptionOpens, 0); |
There was a problem hiding this comment.
Nothing in the registry can increment this counter, so the assertion cannot fail. The child-process test and the stdio-server test already prove no-attach; this one and the last openSessionSubscriptionOnce string can go.
|
|
||
| function validateNewSessionParams(params: NewSessionRequest): void { | ||
| assertBoundedAbsoluteCwd(params.cwd); | ||
| if (params.mcpServers.length > 0) { |
There was a problem hiding this comment.
Fine for PR2. For PR3: the ACP schema makes mcpServers required, so any editor with a context server configured lands here. One real session/new from Zed would settle what to do with it.
Summary
session/newandsession/listthrough the official SDK, including canonicalcwdfiltering, legacy-record omission, stable Runtime Host error mapping, and explicit rejection of unsupported MCP server and additional-directory inputs.initializeand unsupported methods remain Host-independent.session/closeunadvertised and returning-32601; durable Runtime Host Sessions are never deleted or archived by adapter cleanup.Refs #3132
Builds on #3658 (merged)
Review alignment
session/prompt, immediately beforeturn.start; remove the premature one-shot subscription API and client-side subscription-capacity tracking from PR2.revision,cursor, and normalizedcwd; cursor versioning is deferred until there is a concrete compatibility requirement.revision_changedoccurs on its first page as well as on continuation pages.maka --helpto name the methods available in this stage.PR1 alignment
apache/maka:mainafter feat(cli): add ACP stdio server foundation #3658 merged.initializecontract.Verification
npm run build:test— passed.npm run typecheck— passed.npm run lint— passed.npm run format:check— passed.git diff --check— passed.AI use
Select exactly one:
Tool(s) and scope: Codex assisted with implementation, tests, rebase/conflict resolution, verification, and review. Every PR2 commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?